Makefile: Make all builds explicitly fully static (disable CGO) by JoeKar · Pull Request #3466 · micro-editor/micro

JoeKar · 2024-09-11T20:04:00Z

Due to this the "Linux 64 fully static" has been marked deprecated and is only kept for compatibility with:
https://github.com/benweissmann/getmic.ro/blob/f90870e948afab8be9ec40884050044b59ed5b7c/index.sh#L197-L204

Fixes #3463

dmaluka · 2024-09-11T21:00:00Z

Shouldn't we just set CGO_ENABLED=0 in Makefile instead?
We don't want unnecessary divergence between released binaries and binaries built manually by the user?

JoeKar · 2024-09-12T16:22:53Z

Hm, exactly this was my intention in the first place to keep the capability to link it dynamically.
But since the Makefile is indeed the smarter place to do this change we can add a new build target too keep both use cases.

dmaluka · 2024-09-12T17:56:24Z

tools/cross-compile.sh

-echo "Linux 64 fully static"
-CGO_ENABLED=0 GOOS=linux GOARCH=amd64 make build
+echo "Linux 64 fully static (deprecated)"
+# Only kept for https://github.com/benweissmann/getmic.ro/blob/f90870e948afab8be9ec40884050044b59ed5b7c/index.sh#L197-L204


Shouldn't we rather ask @benweissmann to update the script?

I directly provided the PR: benweissmann/getmic.ro#40
But we've to keep in mind, that this will take effect earliest with the upcoming release.

But we've to keep in mind, that this will take effect earliest with the upcoming release.

Sh*t... so this means that if we remove linux64-static and merge benweissmann/getmic.ro#40, getmic.ro will be broken for Musl until the next release?

I would suggest doing one release where you publish statically-linked binaries using both the old name and new name (i.e., have the same statically linked binary published both with and without the -static suffix). Then, I can update getmic.ro after that release is published to cut over to the new name seamlessly, and future releases can drop the -static suffix.

Makefile

dmaluka · 2024-09-12T21:01:38Z

Makefile

+	CGO_ENABLED=0 go build -trimpath -ldflags "-s -w $(GOVARS) $(ADDITIONAL_GO_LINKER_FLAGS)" ./cmd/micro
+
+build-dynamic: generate
+	CGO_ENABLED=1 go build -trimpath -ldflags "-s -w $(GOVARS) $(ADDITIONAL_GO_LINKER_FLAGS)" ./cmd/micro


Seems a bit messy, and "asymmetric" (no quick version of build-dynamic, no dynamic version of build-dbg). Maybe instead of adding a target, just add a variable, e.g. DYNAMIC? I.e. just:

build-quick: CGO_ENABLED=$(DYNAMIC) go build ... build-dbg: CGO_ENABLED=$(DYNAMIC) go build ...

And then we can run DYNAMIC=1 make build or whatever.

Also need to update (i.e. rewrite) the Fully static binary section in README.

Damn, this "hidden" darwin dependency tools/info-plist.go 🤦‍♂️
So we've to exclude darwin somehow or can't generalize it in the Makefile.

Maybe define ADDITIONAL_GO_LINKER_FLAGS to be empty and extend it only in case the GOOS is darwin and forcing CGO_ENABLED to be 1 afterwards. 🤔

Yeah, my first thought was: what the hell is this plist and can we just get rid of it or its -linkmode external. But it is not obvious if it is possible and how, and I have no wish to figure that out.

Maybe define ADDITIONAL_GO_LINKER_FLAGS to be empty and extend it only in case the GOOS is darwin and forcing CGO_ENABLED to be 1 afterwards. 🤔

Seems to make sense.

BTW found its roots: #513

Maybe define ADDITIONAL_GO_LINKER_FLAGS to be empty and extend it only in case the GOOS is darwin and forcing CGO_ENABLED to be 1 afterwards. 🤔

Should do the trick (for) now.
But to be honest, this kind of meta info is only included in the moment micro is natively build on macOS. It's completely missing, at least since the builds are cross compiled via GitHub actions, in our prebuilt binaries. 🤷‍♂️

Yeah, I've just realized that, after looking at GOHOSTOS in your patch.

Which makes this feature from #513 even more... interesting.

Although before 6945aa3 this feature was used for cross-compiled binaries as well... but I doubt it was really working correctly.

Now, should we document this (the fact that MacOS builds are an exception, but only native ones) in README as well?

Yep, better to document before someone is complaining.

Although before 6945aa3 this feature was used for cross-compiled binaries as well... but I doubt it was really working correctly.

Me too, since building micro with darwin AND CGO_ENABLED=1 requires clang.
We could try it with GOOS instead of GOHOSTOS as switch, but then we most probably run into trouble cross compiling for darwin where clang is not available. This might hit the GitHub actions etc.pp

This will keep the symbol table and the DWARF information.

… CGO)

…x 64" It is kept for the next release only to support... https://github.com/benweissmann/getmic.ro/blob/f90870e948afab8be9ec40884050044b59ed5b7c/index.sh#L197-L204 ...and allow a fluent switch via: benweissmann/getmic.ro#40

JoeKar force-pushed the fix/linux-dynamic2static branch from 3b296b1 to 8fcaa38 Compare September 12, 2024 17:09

JoeKar changed the title ~~tools/cross-compile: Make all builds explicitly fully static (disable CGO)~~ Makefile: Make all builds explicitly fully static (disable CGO) Sep 12, 2024

dmaluka reviewed Sep 12, 2024

View reviewed changes

JoeKar added this to the v2.0.15 milestone Sep 12, 2024

JoeKar mentioned this pull request Sep 12, 2024

index.sh: Remove linux64-static override benweissmann/getmic.ro#40

Merged

3 tasks

JoeKar force-pushed the fix/linux-dynamic2static branch from 8fcaa38 to e44f6a2 Compare September 12, 2024 18:56

dmaluka reviewed Sep 12, 2024

View reviewed changes

JoeKar added 4 commits September 16, 2024 22:43

Makefile: Simplify build-tags build target

4d2ddc7

Makefile: Remove "-s -w" from build-dbg target

6e46ae3

This will keep the symbol table and the DWARF information.

Makefile: Make all builds explicitly fully static by default (disable…

90525a6

… CGO)

JoeKar force-pushed the fix/linux-dynamic2static branch 2 times, most recently from 58033b8 to 8257bde Compare September 17, 2024 19:23

JoeKar added 2 commits September 18, 2024 19:08

Makefile: Fix native darwin/macOS builds with forced CGO

fcc7421

README: Remove superflous whitespace

3f1e5ea

JoeKar force-pushed the fix/linux-dynamic2static branch from 8257bde to 3f1e5ea Compare September 18, 2024 17:11

dmaluka approved these changes Sep 18, 2024

View reviewed changes

JoeKar merged commit 71da59f into micro-editor:master Sep 19, 2024

JoeKar deleted the fix/linux-dynamic2static branch September 19, 2024 15:27

Comments

Conversation

JoeKar commented Sep 11, 2024

Uh oh!

dmaluka commented Sep 11, 2024

Uh oh!

JoeKar commented Sep 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmaluka Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmaluka Sep 12, 2024 •

edited

Loading